Skip to content

Fix some mistakes #10711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix some mistakes #10711

wants to merge 1 commit into from

Conversation

ottaviano
Copy link
Contributor

No description provided.

@ottaviano ottaviano force-pushed the fix-some-mistakes branch 2 times, most recently from 1956d54 to bf965dc Compare November 27, 2018 23:45
@@ -6,7 +6,7 @@ How to Work with Lifecycle Callbacks

Sometimes, you need to perform an action right before or after an entity
is inserted, updated, or deleted. These types of actions are known as "lifecycle"
callbacks, as they're callback methods that you need to execute during different
callbacks, as they're callback functions that you need to execute during different
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the example given after the text it is method and not functions.

@@ -7,26 +7,26 @@ on your object. If you're using validation with forms, this means that you
can make these custom errors display next to a specific field, instead of
simply at the top of your form.

This process works by specifying one or more *callback* methods, each of
which will be called during the validation process. Each of those methods
This process works by specifying one or more *callback* functions, each of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I think this all was fine and that method should stay

@@ -88,10 +88,10 @@ Configuration
}
}
The Callback Method
-------------------
The Callback Function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, its method

Copy link
Contributor

@kunicmarko20 kunicmarko20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few minor comments, looks good!

@ottaviano
Copy link
Contributor Author

hi @kunicmarko20, thanks for review.
IMO one callback is always a function, even if it has this format[$object, 'methodName'].
For me callback method indicates rather that we expect a callback in the format [$object, 'methodName'] and not in trim for exemple.

@kunicmarko20
Copy link
Contributor

kunicmarko20 commented Nov 29, 2018

 .. code-block:: php-annotations
 
     /**
      * @ORM\Entity()
      * @ORM\HasLifecycleCallbacks()
      */
     class Product
     {
         // ...
     }
 
 Now, you can tell Doctrine to execute a method on any of the available lifecycle
 events. For example, suppose you want to set a ``createdAt`` date column to
 the current date, only when the entity is first persisted (i.e. inserted):
 
 .. configuration-block::
 
     .. code-block:: php-annotations
 
         // src/Entity/Product.php
 
         /**
          * @ORM\PrePersist
          */
         public function setCreatedAtValue()
         {
             $this->createdAt = new \DateTime();
         }
 
     .. code-block:: yaml
 
         # config/doctrine/Product.orm.yml
         App\Entity\Product:
             type: entity
             # ...
             lifecycleCallbacks:
                 prePersist: [setCreatedAtValue]

This is the example I was talking about, so setCreatedAtValue is the callback here and it is clearly a method and not a function?

So you would say if it was like prePersist: [$product, setCreatedAtValue] then it would be a method and not a function?

I disagree with this but I am not a decider, will leave it to the symfony docs team.

btw, thank you for the contribution! 😄

@@ -142,7 +142,7 @@ With this configuration, there are three validation groups:
``registration``
Contains the constraints on the ``email`` and ``password`` fields only.

Constraints in the ``Default`` group of a class are the constraints that have
Constraints in the ``User`` group of a class are the constraints that have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks wrong

Copy link
Contributor Author

@ottaviano ottaviano Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @OskarStark

// User class
/** @Asert\NotBlanc(groups={"User"}) */

this constraint is not added to the Default group, so
... or that are configured to a group equal to the class name ... is valide only for User and not for Default group.

@@ -1945,7 +1945,7 @@ the ``dev`` environment).
given the adapter they are based on. Internally, a pool wraps the definition
of an adapter.

.. _reference-cache-systen:
.. _reference-cache-system:
Copy link
Contributor

@OskarStark OskarStark Dec 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, but shouldn’t this produce an error by Sphinx on build ?

cc @javiereguiluz

@ottaviano ottaviano changed the base branch from master to 3.4 February 13, 2019 20:21
@ottaviano
Copy link
Contributor Author

I rebased the branch and changed thre PR base to 3.4.

@javiereguiluz
Copy link
Member

@ottaviano thanks for this contribution! Sadly this PR contains too many changes: some are obvious, others have been challenged by reviewers and others don't look right (like the form help related changes, which were introduced in 4.x, not 3.4). So, I've created #11109 with the obvious fixes you did here and we can create in the future other PRs to change other things. Thank you!

javiereguiluz added a commit that referenced this pull request Mar 8, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

Fixed some misc issues

This replaces #10711 making only the changes that are clearly needed.

Commits
-------

1b7318c Fixed some misc issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants